Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

tls: refactor to avoid unsafe array iteration #36772

Merged
merged 1 commit into from
Jan 11, 2021
Merged

tls: refactor to avoid unsafe array iteration #36772

merged 1 commit into from
Jan 11, 2021

Conversation

aduh95
Copy link
Contributor

@aduh95 aduh95 commented Jan 4, 2021

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added the tls Issues and PRs related to the tls subsystem. label Jan 4, 2021
Trott
Trott approved these changes Jan 5, 2021
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM if there are no benchmark surprises

@aduh95 aduh95 added the request-ci Add this label to start a Jenkins CI on a PR. label Jan 5, 2021
@aduh95
Copy link
Contributor Author

aduh95 commented Jan 5, 2021

Benchmark CI: https://ci.nodejs.org/view/Node.js%20benchmark/job/benchmark-node-micro-benchmarks/878/ (queued)

@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jan 5, 2021
@nodejs-github-bot

This comment has been minimized.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/35290/

@aduh95
Copy link
Contributor Author

aduh95 commented Jan 5, 2021

Benchmark didn't show any significant perf regression or improvements.

                                                                                                      confidence improvement accuracy (*)     (**)     (***)
 tls/convertprotocols.js n=1                                                                                         -1.41 %       ±5.18%   ±6.89%    ±8.96%
 tls/convertprotocols.js n=50000                                                                                     -1.44 %       ±5.07%   ±6.74%    ±8.77%
 tls/secure-pair.js size=100 securing='clear' dur=5                                                            *      7.60 %       ±7.28%   ±9.69%   ±12.61%
 tls/secure-pair.js size=100 securing='SecurePair' dur=5                                                             -1.26 %       ±6.73%   ±8.95%   ±11.65%
 tls/secure-pair.js size=100 securing='TLSSocket' dur=5                                                              -1.71 %       ±5.41%   ±7.20%    ±9.38%
 tls/secure-pair.js size=1024 securing='clear' dur=5                                                                 -1.56 %      ±13.08%  ±17.41%   ±22.66%
 tls/secure-pair.js size=1024 securing='SecurePair' dur=5                                                      *     -7.23 %       ±5.52%   ±7.34%    ±9.57%
 tls/secure-pair.js size=1024 securing='TLSSocket' dur=5                                                             -0.77 %       ±6.98%   ±9.29%   ±12.09%
 tls/secure-pair.js size=1048576 securing='clear' dur=5                                                              -0.27 %       ±5.69%   ±7.57%    ±9.87%
 tls/secure-pair.js size=1048576 securing='SecurePair' dur=5                                                         -0.79 %       ±6.73%   ±8.95%   ±11.65%
 tls/secure-pair.js size=1048576 securing='TLSSocket' dur=5                                                           3.25 %       ±8.84%  ±11.79%   ±15.41%
 tls/throughput-c2s.js size=100 type='asc' dur=5                                                                     -3.33 %       ±6.56%   ±8.74%   ±11.39%
 tls/throughput-c2s.js size=100 type='buf' dur=5                                                                      8.03 %       ±8.33%  ±11.11%   ±14.49%
 tls/throughput-c2s.js size=100 type='utf' dur=5                                                                     -0.84 %       ±6.50%   ±8.65%   ±11.26%
 tls/throughput-c2s.js size=1024 type='asc' dur=5                                                                     2.79 %       ±5.20%   ±6.92%    ±9.01%
 tls/throughput-c2s.js size=1024 type='buf' dur=5                                                                     2.79 %       ±5.92%   ±7.88%   ±10.27%
 tls/throughput-c2s.js size=1024 type='utf' dur=5                                                                    -3.73 %       ±8.44%  ±11.23%   ±14.62%
 tls/throughput-c2s.js size=1048576 type='asc' dur=5                                                           *    -12.33 %      ±10.05%  ±13.37%   ±17.41%
 tls/throughput-c2s.js size=1048576 type='buf' dur=5                                                                 -9.29 %      ±10.61%  ±14.12%   ±18.40%
 tls/throughput-c2s.js size=1048576 type='utf' dur=5                                                                  2.11 %      ±11.33%  ±15.08%   ±19.63%
 tls/throughput-c2s.js size=16777216 type='asc' dur=5                                                                -0.08 %       ±2.83%   ±3.76%    ±4.90%
 tls/throughput-c2s.js size=16777216 type='buf' dur=5                                                                -3.87 %       ±5.24%   ±6.97%    ±9.10%
 tls/throughput-c2s.js size=16777216 type='utf' dur=5                                                                 0.97 %       ±3.80%   ±5.06%    ±6.59%
 tls/throughput-c2s.js size=4194304 type='asc' dur=5                                                                  0.53 %       ±3.14%   ±4.18%    ±5.45%
 tls/throughput-c2s.js size=4194304 type='buf' dur=5                                                           *     -8.85 %       ±7.66%  ±10.22%   ±13.34%
 tls/throughput-c2s.js size=4194304 type='utf' dur=5                                                                 -2.66 %       ±4.17%   ±5.56%    ±7.26%
 tls/throughput-s2c.js recvbufgenfn='false' recvbuflen=0 sendchunklen=131072 type='asc' dur=5                        -6.02 %       ±8.70%  ±11.58%   ±15.08%
 tls/throughput-s2c.js recvbufgenfn='false' recvbuflen=0 sendchunklen=131072 type='buf' dur=5                        -2.32 %       ±9.63%  ±12.82%   ±16.70%
 tls/throughput-s2c.js recvbufgenfn='false' recvbuflen=0 sendchunklen=131072 type='utf' dur=5                        -5.18 %      ±10.62%  ±14.13%   ±18.40%
 tls/throughput-s2c.js recvbufgenfn='false' recvbuflen=0 sendchunklen=16777216 type='asc' dur=5                       0.23 %       ±3.36%   ±4.49%    ±5.86%
 tls/throughput-s2c.js recvbufgenfn='false' recvbuflen=0 sendchunklen=16777216 type='buf' dur=5                       2.46 %       ±5.15%   ±6.85%    ±8.92%
 tls/throughput-s2c.js recvbufgenfn='false' recvbuflen=0 sendchunklen=16777216 type='utf' dur=5                      -0.70 %       ±2.74%   ±3.65%    ±4.76%
 tls/throughput-s2c.js recvbufgenfn='false' recvbuflen=0 sendchunklen=256 type='asc' dur=5                           -0.32 %       ±4.42%   ±5.88%    ±7.65%
 tls/throughput-s2c.js recvbufgenfn='false' recvbuflen=0 sendchunklen=256 type='buf' dur=5                            0.63 %       ±4.23%   ±5.63%    ±7.33%
 tls/throughput-s2c.js recvbufgenfn='false' recvbuflen=0 sendchunklen=256 type='utf' dur=5                           -3.61 %      ±11.28%  ±15.01%   ±19.54%
 tls/throughput-s2c.js recvbufgenfn='false' recvbuflen=0 sendchunklen=32768 type='asc' dur=5                         -7.63 %      ±10.69%  ±14.23%   ±18.52%
 tls/throughput-s2c.js recvbufgenfn='false' recvbuflen=0 sendchunklen=32768 type='buf' dur=5                         -1.01 %      ±10.03%  ±13.35%   ±17.38%
 tls/throughput-s2c.js recvbufgenfn='false' recvbuflen=0 sendchunklen=32768 type='utf' dur=5                          0.73 %      ±12.08%  ±16.07%   ±20.91%
 tls/throughput-s2c.js recvbufgenfn='false' recvbuflen=1048576 sendchunklen=131072 type='asc' dur=5                  -1.07 %       ±2.31%   ±3.08%    ±4.02%
 tls/throughput-s2c.js recvbufgenfn='false' recvbuflen=1048576 sendchunklen=131072 type='buf' dur=5                   1.73 %       ±2.17%   ±2.89%    ±3.76%
 tls/throughput-s2c.js recvbufgenfn='false' recvbuflen=1048576 sendchunklen=131072 type='utf' dur=5                   0.59 %       ±2.33%   ±3.10%    ±4.04%
 tls/throughput-s2c.js recvbufgenfn='false' recvbuflen=1048576 sendchunklen=16777216 type='asc' dur=5                 0.91 %       ±2.31%   ±3.07%    ±4.00%
 tls/throughput-s2c.js recvbufgenfn='false' recvbuflen=1048576 sendchunklen=16777216 type='buf' dur=5                 1.30 %       ±2.09%   ±2.79%    ±3.64%
 tls/throughput-s2c.js recvbufgenfn='false' recvbuflen=1048576 sendchunklen=16777216 type='utf' dur=5                -0.24 %       ±1.29%   ±1.72%    ±2.24%
 tls/throughput-s2c.js recvbufgenfn='false' recvbuflen=1048576 sendchunklen=256 type='asc' dur=5                      0.15 %       ±1.70%   ±2.28%    ±2.98%
 tls/throughput-s2c.js recvbufgenfn='false' recvbuflen=1048576 sendchunklen=256 type='buf' dur=5                      0.34 %       ±1.66%   ±2.21%    ±2.87%
 tls/throughput-s2c.js recvbufgenfn='false' recvbuflen=1048576 sendchunklen=256 type='utf' dur=5                      1.22 %       ±2.45%   ±3.28%    ±4.32%
 tls/throughput-s2c.js recvbufgenfn='false' recvbuflen=1048576 sendchunklen=32768 type='asc' dur=5                   -0.24 %       ±2.10%   ±2.80%    ±3.65%
 tls/throughput-s2c.js recvbufgenfn='false' recvbuflen=1048576 sendchunklen=32768 type='buf' dur=5                    0.61 %       ±1.39%   ±1.85%    ±2.41%
 tls/throughput-s2c.js recvbufgenfn='false' recvbuflen=1048576 sendchunklen=32768 type='utf' dur=5                   -2.29 %       ±2.37%   ±3.15%    ±4.11%
 tls/throughput-s2c.js recvbufgenfn='false' recvbuflen=65536 sendchunklen=131072 type='asc' dur=5                     0.50 %       ±2.57%   ±3.42%    ±4.46%
 tls/throughput-s2c.js recvbufgenfn='false' recvbuflen=65536 sendchunklen=131072 type='buf' dur=5                    -1.67 %       ±2.07%   ±2.75%    ±3.58%
 tls/throughput-s2c.js recvbufgenfn='false' recvbuflen=65536 sendchunklen=131072 type='utf' dur=5                     1.04 %       ±2.44%   ±3.25%    ±4.24%
 tls/throughput-s2c.js recvbufgenfn='false' recvbuflen=65536 sendchunklen=16777216 type='asc' dur=5                  -1.19 %       ±2.24%   ±2.98%    ±3.88%
 tls/throughput-s2c.js recvbufgenfn='false' recvbuflen=65536 sendchunklen=16777216 type='buf' dur=5                  -1.31 %       ±1.85%   ±2.46%    ±3.20%
 tls/throughput-s2c.js recvbufgenfn='false' recvbuflen=65536 sendchunklen=16777216 type='utf' dur=5                  -0.07 %       ±0.84%   ±1.11%    ±1.45%
 tls/throughput-s2c.js recvbufgenfn='false' recvbuflen=65536 sendchunklen=256 type='asc' dur=5                        0.05 %       ±2.72%   ±3.63%    ±4.72%
 tls/throughput-s2c.js recvbufgenfn='false' recvbuflen=65536 sendchunklen=256 type='buf' dur=5                        1.38 %       ±2.08%   ±2.78%    ±3.63%
 tls/throughput-s2c.js recvbufgenfn='false' recvbuflen=65536 sendchunklen=256 type='utf' dur=5                       -0.49 %       ±2.13%   ±2.83%    ±3.68%
 tls/throughput-s2c.js recvbufgenfn='false' recvbuflen=65536 sendchunklen=32768 type='asc' dur=5                     -0.38 %       ±2.09%   ±2.79%    ±3.63%
 tls/throughput-s2c.js recvbufgenfn='false' recvbuflen=65536 sendchunklen=32768 type='buf' dur=5                      1.41 %       ±2.46%   ±3.27%    ±4.27%
 tls/throughput-s2c.js recvbufgenfn='false' recvbuflen=65536 sendchunklen=32768 type='utf' dur=5                      0.06 %       ±2.27%   ±3.03%    ±3.94%
 tls/throughput-s2c.js recvbufgenfn='true' recvbuflen=0 sendchunklen=131072 type='asc' dur=5                          7.79 %      ±12.04%  ±16.02%   ±20.86%
 tls/throughput-s2c.js recvbufgenfn='true' recvbuflen=0 sendchunklen=131072 type='buf' dur=5                         11.49 %      ±11.79%  ±15.72%   ±20.51%
 tls/throughput-s2c.js recvbufgenfn='true' recvbuflen=0 sendchunklen=131072 type='utf' dur=5                          3.90 %       ±9.75%  ±12.97%   ±16.88%
 tls/throughput-s2c.js recvbufgenfn='true' recvbuflen=0 sendchunklen=16777216 type='asc' dur=5                       -1.38 %       ±3.22%   ±4.29%    ±5.59%
 tls/throughput-s2c.js recvbufgenfn='true' recvbuflen=0 sendchunklen=16777216 type='buf' dur=5                        1.24 %       ±4.68%   ±6.23%    ±8.11%
 tls/throughput-s2c.js recvbufgenfn='true' recvbuflen=0 sendchunklen=16777216 type='utf' dur=5                        1.18 %       ±3.27%   ±4.36%    ±5.69%
 tls/throughput-s2c.js recvbufgenfn='true' recvbuflen=0 sendchunklen=256 type='asc' dur=5                             2.60 %       ±5.65%   ±7.52%    ±9.79%
 tls/throughput-s2c.js recvbufgenfn='true' recvbuflen=0 sendchunklen=256 type='buf' dur=5                            -3.00 %       ±4.34%   ±5.78%    ±7.55%
 tls/throughput-s2c.js recvbufgenfn='true' recvbuflen=0 sendchunklen=256 type='utf' dur=5                             1.41 %      ±11.39%  ±15.15%   ±19.72%
 tls/throughput-s2c.js recvbufgenfn='true' recvbuflen=0 sendchunklen=32768 type='asc' dur=5                           4.70 %      ±11.22%  ±14.93%   ±19.44%
 tls/throughput-s2c.js recvbufgenfn='true' recvbuflen=0 sendchunklen=32768 type='buf' dur=5                          11.06 %      ±11.64%  ±15.49%   ±20.18%
 tls/throughput-s2c.js recvbufgenfn='true' recvbuflen=0 sendchunklen=32768 type='utf' dur=5                          -6.94 %      ±10.44%  ±13.89%   ±18.09%
 tls/throughput-s2c.js recvbufgenfn='true' recvbuflen=1048576 sendchunklen=131072 type='asc' dur=5             *     -2.36 %       ±2.34%   ±3.12%    ±4.09%
 tls/throughput-s2c.js recvbufgenfn='true' recvbuflen=1048576 sendchunklen=131072 type='buf' dur=5             *      2.59 %       ±2.17%   ±2.89%    ±3.76%
 tls/throughput-s2c.js recvbufgenfn='true' recvbuflen=1048576 sendchunklen=131072 type='utf' dur=5                   -0.21 %       ±2.55%   ±3.40%    ±4.42%
 tls/throughput-s2c.js recvbufgenfn='true' recvbuflen=1048576 sendchunklen=16777216 type='asc' dur=5                  1.02 %       ±2.44%   ±3.25%    ±4.23%
 tls/throughput-s2c.js recvbufgenfn='true' recvbuflen=1048576 sendchunklen=16777216 type='buf' dur=5                  0.70 %       ±2.44%   ±3.25%    ±4.23%
 tls/throughput-s2c.js recvbufgenfn='true' recvbuflen=1048576 sendchunklen=16777216 type='utf' dur=5                  1.07 %       ±1.42%   ±1.90%    ±2.47%
 tls/throughput-s2c.js recvbufgenfn='true' recvbuflen=1048576 sendchunklen=256 type='asc' dur=5                      -0.67 %       ±2.65%   ±3.52%    ±4.58%
 tls/throughput-s2c.js recvbufgenfn='true' recvbuflen=1048576 sendchunklen=256 type='buf' dur=5                      -0.43 %       ±1.69%   ±2.25%    ±2.93%
 tls/throughput-s2c.js recvbufgenfn='true' recvbuflen=1048576 sendchunklen=256 type='utf' dur=5                      -0.54 %       ±2.88%   ±3.83%    ±4.99%
 tls/throughput-s2c.js recvbufgenfn='true' recvbuflen=1048576 sendchunklen=32768 type='asc' dur=5                    -0.26 %       ±2.12%   ±2.82%    ±3.67%
 tls/throughput-s2c.js recvbufgenfn='true' recvbuflen=1048576 sendchunklen=32768 type='buf' dur=5                     0.18 %       ±1.69%   ±2.25%    ±2.93%
 tls/throughput-s2c.js recvbufgenfn='true' recvbuflen=1048576 sendchunklen=32768 type='utf' dur=5                    -0.57 %       ±2.56%   ±3.41%    ±4.44%
 tls/throughput-s2c.js recvbufgenfn='true' recvbuflen=65536 sendchunklen=131072 type='asc' dur=5               *      2.31 %       ±2.02%   ±2.69%    ±3.52%
 tls/throughput-s2c.js recvbufgenfn='true' recvbuflen=65536 sendchunklen=131072 type='buf' dur=5                     -0.75 %       ±1.78%   ±2.37%    ±3.08%
 tls/throughput-s2c.js recvbufgenfn='true' recvbuflen=65536 sendchunklen=131072 type='utf' dur=5                     -1.48 %       ±2.65%   ±3.53%    ±4.60%
 tls/throughput-s2c.js recvbufgenfn='true' recvbuflen=65536 sendchunklen=16777216 type='asc' dur=5                    1.08 %       ±2.54%   ±3.38%    ±4.41%
 tls/throughput-s2c.js recvbufgenfn='true' recvbuflen=65536 sendchunklen=16777216 type='buf' dur=5                   -1.48 %       ±2.26%   ±3.02%    ±3.93%
 tls/throughput-s2c.js recvbufgenfn='true' recvbuflen=65536 sendchunklen=16777216 type='utf' dur=5                    0.92 %       ±1.65%   ±2.21%    ±2.89%
 tls/throughput-s2c.js recvbufgenfn='true' recvbuflen=65536 sendchunklen=256 type='asc' dur=5                         1.95 %       ±2.02%   ±2.70%    ±3.52%
 tls/throughput-s2c.js recvbufgenfn='true' recvbuflen=65536 sendchunklen=256 type='buf' dur=5                        -0.58 %       ±2.21%   ±2.94%    ±3.82%
 tls/throughput-s2c.js recvbufgenfn='true' recvbuflen=65536 sendchunklen=256 type='utf' dur=5                        -0.37 %       ±1.32%   ±1.77%    ±2.33%
 tls/throughput-s2c.js recvbufgenfn='true' recvbuflen=65536 sendchunklen=32768 type='asc' dur=5                      -0.96 %       ±2.15%   ±2.85%    ±3.72%
 tls/throughput-s2c.js recvbufgenfn='true' recvbuflen=65536 sendchunklen=32768 type='buf' dur=5                       0.06 %       ±1.87%   ±2.48%    ±3.23%
 tls/throughput-s2c.js recvbufgenfn='true' recvbuflen=65536 sendchunklen=32768 type='utf' dur=5                       1.41 %       ±2.24%   ±2.98%    ±3.89%
 tls/tls-connect.js dur=5 concurrency=1                                                                             -21.57 %      ±53.34% ±121.33%  ±377.99%
 tls/tls-connect.js dur=5 concurrency=10                                                                             -5.98 %      ±91.53% ±405.44% ±3394.79%

Be aware that when doing many comparisons the risk of a false-positive
result increases. In this case there are 100 comparisons, you can thus
expect the following amount of false-positive results:
  5.00 false positives, when considering a   5% risk acceptance (*, **, ***),
  1.00 false positives, when considering a   1% risk acceptance (**, ***),
  0.10 false positives, when considering a 0.1% risk acceptance (***)

@aduh95 aduh95 added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 5, 2021
@aduh95
tls: refactor to avoid unsafe array iteration
307b79d 
PR-URL: nodejs#36772
Reviewed-By: Rich Trott <rtrott@gmail.com>
@aduh95
Copy link
Contributor Author

aduh95 commented Jan 11, 2021

Landed in 307b79d

@aduh95 aduh95 merged commit 307b79d into nodejs:master Jan 11, 2021
@aduh95 aduh95 deleted the tls-array-iteration branch January 11, 2021 11:45
danielleadams pushed a commit that referenced this pull request Jan 12, 2021
@aduh95 @danielleadams
tls: refactor to avoid unsafe array iteration
c23cca2 
PR-URL: #36772
Reviewed-By: Rich Trott <rtrott@gmail.com>
@danielleadams danielleadams mentioned this pull request Jan 12, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Trott Trott Trott approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. tls Issues and PRs related to the tls subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@aduh95 @nodejs-github-bot @Trott @targos